Skip to content

feat(create): fetch remote manifest after linking app during create#585

Open
srtaalej wants to merge 21 commits into
mainfrom
ale-fetch-remote-manifest
Open

feat(create): fetch remote manifest after linking app during create#585
srtaalej wants to merge 21 commits into
mainfrom
ale-fetch-remote-manifest

Conversation

@srtaalej

@srtaalej srtaalej commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Changelog

slack create --app now fetches the app's manifest from remote settings and writes it to the project's manifest.json, replacing the template's generic manifest with the actual app configuration.

Summary

Follow-up to #565. After linking an existing app during slack create --app --template, the CLI now fetches the remote manifest from App Settings and overwrites the template's manifest.json with it.

This also updates LinkExistingApp to return (*types.SlackAuth, error) so callers can use the auth token for subsequent API calls (like manifest fetching).

Design decisions:

  • Manifest source remains local in .slack/config.json — the remote content is written locally
  • Only manifest.json is overwritten (not manifest.ts/.js which are code files in Deno projects)
  • On fetch failure, a warning is printed and the command continues — the project and link still succeed

Testing

make test testdir=cmd/project testname=TestCreateCommand_AppFlag_FetchesRemoteManifest
make test testdir=cmd/project testname=TestCreateCommand_AppFlag
make test testdir=cmd/project testname=Test_Project_InitCommand
make test testdir=cmd/app testname=TestLinkCommand

Manual:

  1. make build
  2. ./bin/slack create my-test -t slack-samples/bolt-js-starter-template --app <real-app-id> --environment local
  3. Verify my-test/manifest.json contains the remote app's manifest
  4. Test with an invalid app ID to confirm the warning is shown and the project is still created

Notes 🔴

  • Open question: should we set manifest.source to remote in .slack/config.json after writing the fetched manifest? Currently left as local so the user "owns" the file going forward.

A future PR could add merging logic between template and remote manifests.

  • manifest.ts/.js (Deno SDK) projects are not supported

⌛ not merging until #598 is merged because of conflicts

Requirements

@srtaalej srtaalej self-assigned this Jun 9, 2026
@srtaalej srtaalej requested a review from a team as a code owner June 9, 2026 00:27
@srtaalej srtaalej added enhancement M-T: A feature request for new functionality changelog Use on updates to be included in the release notes semver:minor Use on pull requests to describe the release version increment labels Jun 9, 2026
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 36.36364% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.59%. Comparing base (44ce627) to head (2283fbc).

Files with missing lines Patch % Lines
internal/manifest/export.go 0.00% 17 Missing ⚠️
cmd/project/create.go 75.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #585      +/-   ##
==========================================
- Coverage   71.68%   71.59%   -0.10%     
==========================================
  Files         226      227       +1     
  Lines       19176    19207      +31     
==========================================
+ Hits        13747    13751       +4     
- Misses       4220     4242      +22     
- Partials     1209     1214       +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

srtaalej and others added 3 commits June 12, 2026 13:57
Resolve conflicts by integrating main's stricter --app/--environment
validation with the remote manifest fetch feature.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
@srtaalej srtaalej requested review from mwbrooks and zimeg June 18, 2026 16:50

@zimeg zimeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srtaalej Thanks for sharing these changes 👾 ✨

I'm leaving a few comments on a first run experience around prompts and earlier errors. Other comments are on code organization with thoughts around the difference between create and link logic that might be interesting to clear up ✂️ 📠

Testing is solid for me too but I leave a note around escaped values that are unsettling to me. I'm unsure what caused that 👻

Comment thread cmd/project/create.go Outdated
Comment on lines +310 to +324
// fetchAndWriteRemoteManifest fetches the app manifest from remote settings and writes it to the project.
func fetchAndWriteRemoteManifest(ctx context.Context, clients *shared.ClientFactory, token, appID, projectPath string) error {
slackYaml, err := clients.AppClient().Manifest.GetManifestRemote(ctx, token, appID)
if err != nil {
return err
}
data, err := json.MarshalIndent(slackYaml.AppManifest, "", " ")
if err != nil {
return err
}
data = append(data, '\n')
manifestPath := filepath.Join(projectPath, "manifest.json")
return afero.WriteFile(clients.Fs, manifestPath, data, 0644)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📠 suggestion: We might find a new file better for this? It might be shared in other commands or PRs #591!

internal/manifest/export.go

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 040a3c8 💟

Comment thread cmd/project/create.go
Comment on lines +244 to +250
clients.IO.PrintWarning(ctx, "%s", style.Sectionf(style.TextSection{
Text: "Could not fetch the remote app manifest",
Secondary: []string{
fetchErr.Error(),
"The template manifest was kept unchanged",
},
}))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ question: Could returning the error here bring more confidence for scripts? I'm unsure when this might fail but would hope that invalid app IDs error overall to start

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I'm hesitant because the create+--app command doesn't necessarily fail if fetching remote manifest fails. the app is still created and linked successfully. only one part of the command fails in this case so maybe it would be better to provide a way to try again in follow up PRs?
or simply telling users to run slack manifest info --source remote and copy that into manifest.json 🤔

Comment thread cmd/app/link.go Outdated
LinkAppFooterSection(ctx, clients, app)

return nil
return auth, nil

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔗 question: Instead of returning an auth to calling functions that's often ignored, would setting up the manifest as part of the link command if no apps are saved make sense?

👾 ramble: I'm wanting to avoid mixing concepts with the create command which should be from a project template I think while link is seeming more app specific.

Comment thread cmd/project/create.go Outdated
if err != nil {
return err
}
data, err := json.MarshalIndent(slackYaml.AppManifest, "", " ")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔬 note: I'm finding some values are escaped but this hasn't caused an error when testing the custom step template:

-           "type": "slack#/types/user_id",
+           "type": "slack#\/types\/user_id",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 1092da0 🚀

Comment thread cmd/project/create.go Outdated
}
data = append(data, '\n')
manifestPath := filepath.Join(projectPath, "manifest.json")
return afero.WriteFile(clients.Fs, manifestPath, data, 0644)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏁 suggestion: We should save the hash alongside this to avoid a confusing prompt of a changed manifest on the first "run" command:

hash, err := clients.Config.ProjectConfig.Cache().NewManifestHash(ctx, upstream.Manifest.AppManifest)
if err != nil {
return types.App{}, "", err
}
if !hash.Equals(saved) {
err := clients.Config.ProjectConfig.Cache().SetManifestHash(ctx, app.AppID, hash)
if err != nil {
return types.App{}, "", err
}
}

$ cd my-test
$ slack run

📚 App Manifest
   Manifest values for this app are overwritten on reinstall

┃ Overwrite manifest on app settings with the project's manifest file?
┃   Yes
┃ ❱ No

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in aecb626 🌟

@mwbrooks mwbrooks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answers to Open Questions:

Open question: should we set manifest.source to remote in .slack/config.json after writing the fetched manifest? Currently left as local so the user "owns" the file going forward.

It's a good question - I think we should not change the manifest source. The main reason is that this keeps the existing functionality and it's unclear whether the developer would prefer App Settings or manifest.json when creating an app locally from App Settings.

So, since there's no clear answer, I'd rather keep the functionality unchanged. Instead, we can continue to move forward on the 2-way sync solution to remove this issue entirely.

manifest.ts/.js (Deno SDK) projects are not supported

Totally fine to not support manifest.ts/.js. This is a Deno SDK only feature and App Settings isn't offering Deno SDK creation journeys afaik.

@mwbrooks mwbrooks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 @srtaalej This is shaping up really nice! I've left a suggestion on how we can tighten up the LinkExistingApp function to return the auth while also improving how it currently sets app. While it feels like a larger change, it turns out to be a similar line count and should help align this PR around @zimeg's feedback.

Comment thread cmd/app/link.go Outdated
// link an existing app and additional information is included in the header.
// The shouldConfirm option is encouraged for third-party callers.
func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *types.App, shouldConfirm bool) (err error) {
func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, app *types.App, shouldConfirm bool) (_ *types.SlackAuth, err error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: We should consider returning a single prompts.SelectedApp (contains an app and auth) instead of mixing a pointer out-parameter (app *types.App) with the new returned types.SlackAuth.

The app parameter is currently an "out-param" dressed up as an "in-param" - it's always overwritten at L171 (*app, auth, err = promptExistingApp(...)). Every caller passes a throwaway &types.App{}.

After this PR, app comes out via the pointer while auth comes out via the return value - two different output mechanisms for two values produced at the same call site. This makes the function confusing and brittle.

prompts.SelectedApp already pairs App + Auth and is the exact shape this function produces. Also, cmd/app already imports internal/prompts, so no new dependency.

Suggested signature:

func LinkExistingApp(ctx context.Context, clients *shared.ClientFactory, shouldConfirm bool) (prompts.SelectedApp, error)

A zero SelectedApp{} (i.e. selected.App.AppID == "") signals "user declined at the confirm prompt" - which is what create.go already checks on L243.

Caller changes:

  • LinkCommandRunE (this file): drop the app *types.App param; call _, err := LinkExistingApp(ctx, clients, false). Also remove the throwaway app := &types.App{} in NewLinkCommand.
  • cmd/project/init.go:113: _, err = app.LinkExistingApp(ctx, clients, true)
  • cmd/project/create.go:232: selected, linkErr := app.LinkExistingApp(ctx, clients, false) then if selected.App.AppID != "" { fetchAndWriteRemoteManifest(ctx, clients, selected.Auth.Token, selected.App.AppID, absProjectPath) } — the existing auth != nil && linkedApp.AppID != "" guard collapses to one condition.

Diff size is comparable to the current PR, no callers need to manufacture an empty App{}, and the API answers the obvious question: yes, this function returns a selected app.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for this clarification @mwbrooks! I agree the LinkExistingApp is a bit messy 🤔 since i have another pr that also makes changes to LinkExistingApp, i feel like i should leave that refactor to a follow-up PR to avoid more conflicts. for now i have taken out the auth param i introduced and instead get it from:

@create.go
if linkedApp.AppID != "" {
      auth, err := clients.Auth().AuthWithTeamID(ctx, linkedApp.TeamID)
      if err != nil { return err }
      fetchErr := manifest.FetchAndWriteRemoteManifest(ctx, clients, auth.Token, ...)
      ...

so that link.go remains unchanged. what do you think of this approach?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srtaalej I think your approach to manually get the auth param works and keeps the impact low. So feel free to run with that!

@srtaalej srtaalej marked this pull request as draft June 30, 2026 00:35
@srtaalej srtaalej modified the milestones: v4.4.0, Next Release Jun 30, 2026
@srtaalej

Copy link
Copy Markdown
Contributor Author

Update

The diff is much smaller now:

  • FetchAndWriteRemoteManifest was moved to its own file (internal/manifest/export.go) so it can be reused by other commands (e.g. feat: add slack manifest diff command #591)
  • The LinkExistingApp signature is unchanged from main — no auth return was added. Instead, create.go resolves the token via clients.Auth().AuthWithTeamID() after linking
  • Fixed forward slash escaping in the exported manifest JSON
  • Saved the manifest hash after writing so the first slack run doesn't prompt about a "changed manifest"

Follow-ups

  • Easier recovery for failed manifest sync: Currently there's no CLI command to retry fetching the remote manifest after create finishes (no slack manifest pull). For now users can run slack manifest info --source remote > manifest.json as a workaround. A dedicated command would be better.
  • Refactor LinkExistingApp: The app *types.App out-param pattern is awkward (per Michael's feedback). PR feat(create): cleanup app creation outputs when slack create -app passed #598 is already reworking this — will align after that lands.

@srtaalej srtaalej marked this pull request as ready for review June 30, 2026 17:01
@srtaalej srtaalej requested review from mwbrooks and zimeg June 30, 2026 17:03
Comment thread cmd/project/create.go
}
fetchErr := manifest.FetchAndWriteRemoteManifest(ctx, clients, auth.Token, linkedApp.AppID, absProjectPath)
if fetchErr != nil {
clients.IO.PrintWarning(ctx, "%s", style.Sectionf(style.TextSection{

@srtaalej srtaalej Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a comment about this silent fail 🪂 #585 (comment) @zimeg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog Use on updates to be included in the release notes enhancement M-T: A feature request for new functionality semver:minor Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants